-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Value list exception support for all rule types #133254
Conversation
eb75d87
to
5a40fba
Compare
5a40fba
to
796c490
Compare
1e4e90b
to
bd0cb51
Compare
...ecurity_solution/server/lib/detection_engine/routes/exceptions/get_exception_filter_route.ts
Outdated
Show resolved
Hide resolved
9cebeb2
to
75f28fe
Compare
f49bdc6
to
1eb6893
Compare
7b99cca
to
eec67c2
Compare
@elasticmachine merge upstream |
346bb73
to
a9f5bf5
Compare
@elasticmachine merge upstream |
6aa3509
to
5e52a7b
Compare
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. Lots of async
and await
s to remove once getQueryFilter
on the server side is made sync again, I commented on a few but there are a lot so I stopped commenting on all of them.
Code review only so far, will test next.
x-pack/plugins/lists/server/services/items/find_all_list_items.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/services/items/find_all_list_items.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/services/items/find_all_list_items.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/services/items/find_all_list_items.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/services/lists/list_client_types.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/get_query_filter.ts
Outdated
Show resolved
Hide resolved
...curity_solution/public/detections/containers/detection_engine/exceptions/get_query_filter.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/build_events_query.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/executors/threshold.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/get_filter.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! Left a few nits. Could some integration tests be added for the find_list
and find_small_list
route that touch on these use cases?
Integration tests live here: x-pack/test/lists_api_integration/security_and_spaces/tests
...ecurity_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/routes/get_exception_filter_route.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/server/routes/get_exception_filter_route.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done
x-pack/plugins/lists/server/services/exception_lists/build_exception_filter.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
) => { | ||
if (unprocessedExceptions.length > 0) { | ||
const exceptionNames = unprocessedExceptions.map((exception) => exception.name); | ||
ruleExecutionLogger.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use logStatusChange
here instead of warn
so that the warning is written to the rule status SO and shows up in the UI. We also need to return whether or not a warning was written to the security rule type wrapper so it doesn't call logStatusChange
again and overwrite the warning with success
at the end of execution. This can be done as a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for adding the integration tests!
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @dplumlee |
…tions to use parameters (#145889) (#146414) # Backport This will backport the following commits from `main` to `8.5`: - [[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)](#145889) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Michael Olorunnisola","email":"[email protected]"},"sourceCommit":{"committedDate":"2022-11-28T15:08:48Z","message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","backport","release_note:fix","Team:Threat Hunting:Investigations","v8.5.0","v8.6.0","v8.7.0"],"number":145889,"url":"https://github.com/elastic/kibana/pull/145889","mergeCommit":{"message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6"}},"sourceBranch":"main","suggestedTargetBranches":["8.5","8.6"],"targetPullRequestStates":[{"branch":"8.5","label":"v8.5.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145889","number":145889,"mergeCommit":{"message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6"}}]}] BACKPORT--> Co-authored-by: Michael Olorunnisola <[email protected]>
…tions to use parameters (#145889) (#146415) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)](#145889) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Michael Olorunnisola","email":"[email protected]"},"sourceCommit":{"committedDate":"2022-11-28T15:08:48Z","message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","backport","release_note:fix","Team:Threat Hunting:Investigations","v8.5.0","v8.6.0","v8.7.0"],"number":145889,"url":"https://github.com/elastic/kibana/pull/145889","mergeCommit":{"message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6"}},"sourceBranch":"main","suggestedTargetBranches":["8.5","8.6"],"targetPullRequestStates":[{"branch":"8.5","label":"v8.5.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145889","number":145889,"mergeCommit":{"message":"[Security Solution][Investigations][Timeline] - Update getExceptions to use parameters (#145889)\n\n## Summary\r\n\r\nFixes: https://github.com/elastic/kibana/issues/136772\r\n\r\nThe issue was introduced by a couple of changes:\r\n\r\nFirst:\r\nhttps://github.com//pull/136163/files#diff-02d33a1ed6679f7775dc01941ca21b085d7c008ecffe5e029f5967407a5e5b13L23\r\nin 8.4.\r\n\r\nThe bug: A filter on the timeline UI relied on the `exceptions_list`\r\nfield provided on `_source` to auto-generate a filter when investigating\r\nin timeline labelled `Not Exceptions` which would filter out the\r\nexceptions from the timeline. This PR resolves that issue by pulling the\r\n`exceptions_list` field from `kibana.alert.rule.parameters`.\r\n\r\nSecond:\r\nhttps://github.com//pull/133254/files#diff-0f69b69fd9cefef6ed04a048d7df86b7e385e816bdf17309212437dc3f69726cL74\r\n\r\nThe filter actually stopped being passed to timeline entirely because of\r\nthe above change.\r\n\r\nWith the fixes in place:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/17211684/203111748-7a0c2eb5-a46f-4f88-9d77-3628204625ac.mov","sha":"b32c8b9df89188cdcb149bd1d9494d3f99999ad6"}}]}] BACKPORT--> Co-authored-by: Michael Olorunnisola <[email protected]>
Summary
Addresses https://github.com/elastic/security-team/issues/3076 (internal)
Overview
Adds value list exception support to every rule type with a few caveats. Structurally, this PR adds the definition of a "small" list that is able to be included directly in the rule executor's elasticsearch query instead of filtering out large lists in the post-execution process. This allows us to add value list exceptions for every rule type given some constraints, but still keep the post-execution logic and not degrade any current expected functionality.
Definitions
Caveats
Refactoring
This PR also restructures quite a bit of code/packages due to the refactoring of the exception filter builder. By moving the exception builder logic to the backend, we have implemented a new API that returns the results previously done by the common package logic.
Screenshots
Checklist
Delete any items that are not applicable to this PR.
For maintainers